Fix jdbc-v2: assume a function in the values list of a recovered ANTLR4 parse tree - #3028
Conversation
…R4 parse tree A function call in an insert values list is reported by a listener callback on the ANTLR4 parse tree. A statement the grammar cannot match is still given a parse tree, completed by error recovery, which skips the tokens the parser recovered on - a function call among them is never reported, so useFunction stayed false while a function was present. With the beta RowBinary writer enabled such an insert was then routed to it, where a literal function-call column cannot be written. Fixes: #3027
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…4-usefunction-recovered-tree
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…4-usefunction-recovered-tree
|



Description
Fixes #3027.
A function call in an insert values list is reported to
ParsedPreparedStatement.useFunctiononly by a listener callbackon the ANTLR4 parse tree (
insertParameterFuncExpr). A statement the bundled grammar cannot match is still given a parsetree, completed by error recovery, which skips the tokens the parser recovered on: a function call among them is never
reported, so
useFunctionstayedfalsewhile a function was present - e.g.INSERT INTO t (v1, v2) VALUES (?, hex(x'AB')), valid ClickHouse the grammar has no hex string literal for.ConnectionImpl#prepareStatementuses thatflag to decide whether an insert can be written with the beta
RowBinarywriter (which requires a values list ofparameter placeholders only), so with
beta.row_binary_for_simple_insert=truesuch a statement was routed to the writer,where a literal function-call column cannot be written. Since a recovered tree cannot tell whether a function call is
present, both ANTLR4 backends now assume one for an insert that could not be parsed without errors, so the statement
takes the generic parameter substitution path - as it already did for a function call the grammar matches. The default
JAVACCbackend derives function usage from the SQL itself and is not affected.Changes
jdbc-v2/.../internal/SqlParserFacade.java: addedassumeFunctionInValuesListOfRecoveredParseTree(...), called fromthe existing
isHasErrors()branch ofANTLR4Parser#parsePreparedStatementand of the override inANTLR4AndParamsParser(the two entry points that build aParsedPreparedStatementfrom an ANTLR4 parse tree). Itsets
useFunctionfor an insert only - in these backends the flag describes the insert values list.CHANGELOG.md: bug-fix entry, including that the assumption applies to any insert parsed with errors.Test
BaseSqlParserFacadeTest#testInsertUseFunction(runs for all three parser backends): a@DataProviderpinninguseFunctionfor a values list of placeholders only (false), a function call the grammar matches (now(),toString(?)-true), and a function call it cannot match (hex(x'AB')in the first value, in the second value, andin the second of two value groups -
true). Each row also pins whether the statement parses without errors (ANTLR4backends), so the rows keep covering the error-recovery branch. 6 rows fail on
mainforANTLR4andANTLR4_PARAMS_PARSER, none with the fix; theJAVACCrows pass before and after.BaseSqlParserFacadeTest#testUseFunctionOfUnparseableSelect: contrast case - aSELECTthat cannot be parsed keepsreporting no function usage in the ANTLR4 backends (skipped for
JAVACC, which reports any function use).WriterStatementImplTest#testInsertWithUnparseableFunctionNotWrittenWithRowBinary(integration, both ANTLR4backends): through
prepareStatementwith the beta writer enabled,INSERT INTO t (v1, v2) VALUES (?, hex(x'AB'))isnot a
WriterStatementImpland storesv2 = 'AB'; a values list of placeholders only still is aWriterStatementImpland stores its bound value. Both rows fail on
main(routed to the writer).mvn -pl jdbc-v2 test: 1322 tests, 0 failures. Focused integration run (WriterStatementImplTest,PreparedStatementTest): 77 tests, 0 failures.Pre-PR validation gate
prepareStatement)prepareStatement→ConnectionImplrouting)AGENTS.md/docs/changes_checklist.md(new package-private method with thesmallest visibility and focused tests;
debug-level log on a non-hot error path that logs no SQL values;parametrized
@DataProviderinstead of near-identical methods; no public API or configuration change, sodocs/features.mdneeds no update)Related
Same family as #3019 / #3025: parse-tree-derived fields of a recovered ANTLR4 tree. This PR fixes a different field
(
useFunction) with a different consumer (theRowBinaryrouting decision) and is independent of the PRs for those.